Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Add password as command line argument #253

Merged
merged 3 commits into from
Dec 17, 2019

Conversation

jlerche
Copy link

@jlerche jlerche commented Dec 16, 2019

What problem does this PR solve?

Previously it is not possible to supply the user password by command line, unlike other restore tools like loader

What is changed and how it works?

Adds -tidb-password command line option

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the tidb-ansible repository
  • Need to be included in the release note

@claassistantio
Copy link

claassistantio commented Dec 16, 2019

CLA assistant check
All committers have signed the CLA.

@@ -222,6 +222,7 @@ func (cfg *Config) LoadFromGlobal(global *GlobalConfig) error {
cfg.TiDB.Host = global.TiDB.Host
cfg.TiDB.Port = global.TiDB.Port
cfg.TiDB.User = global.TiDB.User
cfg.TiDB.Psw = global.TiDB.Psw

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am more familiar with "Pwd" as an abbreviation, but do we need to abbreviate this at all?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Psw to mimic the field name in the config

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you mean by "mimic the field name in the config"? There is no Psw field in the configuration file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field can be named "Psw" for consistency with old code (or all renamed to "Password"), but the CLI argument should be named -tidb-password not -tidb-psw.

@jlerche jlerche requested a review from kennytm December 17, 2019 00:49
@kennytm
Copy link
Collaborator

kennytm commented Dec 17, 2019

/run-all-tests

Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kennytm kennytm added status/LGT1 One reviewer already commented LGTM (LGTM1) Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Dec 17, 2019
@jlerche
Copy link
Author

jlerche commented Dec 17, 2019

pingcap/docs#1711 PR for updating docs

@gregwebs gregwebs merged commit 61a64ad into pingcap:master Dec 17, 2019
@jlerche jlerche deleted the add_password_to_cli_args branch December 17, 2019 18:06
@kennytm kennytm removed the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT1 One reviewer already commented LGTM (LGTM1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants